Skip to content

Conversation

@JonasHaouzi
Copy link
Contributor

Hi guys.

First of all, a big thank you for the amazing work you did with this library and the Symfony bundle.

Here is a little suggestion related to my Varnish configuration, and I think many others developers are sharing it.

➜ What the library is currently doing
When writing a test and asserting if a request has been "HIT", we assert the header X-Cache is equal to HIT.

➜ Why is it not working in some cases?
Because some Varnish instances are set-up to return "HIT 1", "HIT 2", etc.. the assertions is failing.

➜ What's the proposal?
In this proposal, I modified the code to stop checking that the header is exactly identical to HIT but match a given regular expression. This regular expression allows the word HIT followed by an optional numerical number.

Jonas

@ddeboer
Copy link
Member

ddeboer commented Jul 5, 2016

First of all, a big thank you for the amazing work you did with this library and the Symfony bundle.

You’re welcome! And thank you for taking the time to contribute to this library.

Just to make sure this is a necessary change: in our docs we recommend configuring Varnish to set a simple HIT/MISS value. Can you point us to docs or config that suggest including a counter?

@dbu
Copy link
Contributor

dbu commented Jul 5, 2016

maybe we could just do false !== strpos to be more flexible for custom setups?

@JonasHaouzi
Copy link
Contributor Author

Yes that'd work perfectly for me. Thanks.

@JonasHaouzi
Copy link
Contributor Author

@ddeboer In our case, our VCL was already written and we haven't reuse the ones you're providing. So I can't provide it, sorry.

I found as well the site number 82 in France, and they're using the same:

$ curl --silent -I http://www.leparisien.fr/ | grep HIT
X-VarnCache: HIT 120

If you prefer not to include that, it's ok, we'll create our own assertions methods even if I think the cost is quite low and can benefit to others uses in the same case.

@dbu dbu merged commit 9693721 into FriendsOfSymfony:master Jul 5, 2016
@dbu
Copy link
Contributor

dbu commented Jul 5, 2016

thanks, this makes sense to me, the cost indeed is low and it adds flexibility.

@dbu
Copy link
Contributor

dbu commented Jul 5, 2016

note that you did this PR against master which is the development version of 2.0. if you use 2.0 in production and it works, that is nice but i would recommend to at least lock to a specific commit id in your composer.json, as we might do some BC breaks without warning... see for example #285

@ddeboer
Copy link
Member

ddeboer commented Jul 5, 2016

Thanks @JonasHaouzi!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants